⚠️ IMPORTANT - Generic error logging removed, creating a monitoring gap

The original code called services.errorReporter.log(error: error) for all errors from save operations. The new code only logs the specific ServerError matching "Cipher was not encrypted for the current user".

While ServerError (as a NonLoggableError) and URLError were already filtered out by CrashlyticsErrorReporter, other non-network errors (encryption failures, SDK errors, state service errors) were reaching Crashlytics before and will now be silently dropped.

The targeted BitwardenError.generalError logging should be added alongside the existing generic logging, not replace it:

@claude this is an old version, I already updated this. Review the last changes.

Claude finished @LRNcardozoWDF's task in 1m 32s —— View job

Re-Review of Latest Changes

• Gather context on latest changes
• Review updated code
• Post review findings

Commits reviewed since last review: 87c4115 ([PM-24195] Fix tests), c758640 ([PM-24195] Fix pr comment)

Previous Findings Status

Changes Since Last Review

1. AddEditItemProcessor.swift — Added else branch at line 708 that calls services.errorReporter.log(error: error) for errors not matching the targeted "Cipher was not encrypted for the current user" message. This restores generic error logging for all other errors.

2. AccountTokenProvider.swift — The race condition diagnostic (getActiveAccountId comparison) is now isolated in its own do/catch block (lines 111–122), preventing diagnostic failures from affecting the success path. Error type renamed from TokenRefreshRaceConditionError to AccountTokenProviderError.

3. AccountTokenProviderTests.swift — Added @MainActor, fixed async tearDown, set stateService.activeAccount fixture in setUp, and added test_refreshToken_logsError_whenGetUserIdAfterThrows to verify the inner do/catch isolation works correctly (token is returned even when the diagnostic check throws).

4. AddEditItemProcessorTests.swift — Updated test_perform_savePressed_serverError_otherMessage_doesNotLog to expect 1 logged error (matching restored generic logging).

Verdict

Both previous findings have been properly addressed. No new issues found in the incremental changes. The code is clean, well-tested, and ready to merge.

Approved ✅

🎨 SUGGESTED - Diagnostic code shares the same do block as the main refresh logic

If getActiveAccountId() on line 111 throws (e.g., the active account was removed during the refresh -- precisely the race condition being investigated), the error propagates to the catch block, which calls onRefreshTokenError and re-throws. This would make a successful token refresh appear as a failure to callers, since setTokens() on line 105 already completed.

Consider wrapping the post-refresh diagnostic section in its own do/catch to keep it isolated from the success path:

// After setTokens succeeds:
do {
    let userIdAfter = try await stateService.getActiveAccountId()
    if userIdBefore != userIdAfter {
        let error = TokenRefreshRaceConditionError(
            userIdBefore: userIdBefore,
            userIdAfter: userIdAfter,
        )
        errorReporter.log(error: error)
    }
} catch {
    errorReporter.log(error: error)
}

🤔 Is this the only endpoint that can throw this error? If not, can we extract to some other place more generic?
🎨 Instead of using if case you can use catch with the specific error and avoid the if else:

catch let ServerError.error(errorResponse):

I didn't found other endpoints, I'll look into it again.

🤔 If this would happen then I think we shouldn't be setting the tokens in the wrong account. So you could just add the check before setTokens and add the userId: String? as parameter to setTokens so you can pass the user id you have in there instead of getting it again inside the function:

🤔 Moreover, there could potentially be a race condition between these two as well:

                let userIdBefore = try await stateService.getActiveAccountId()

                let refreshToken = try await tokenService.getRefreshToken()

So perhaps you could add userId: String? to getRefreshToken as well thus you can pass it, so the userId used throughout this refreshToken is as stable as possible.

🤔 I think you can remove the comment if you start passing the user ID to the token service functions. Also I think userIdBefore should be renamed to userIdToRefresh or something like that so it's easier to know what it refers to.

🎨 No need to recreate the server error, you already have it in the error variable.

⛏️ Does it make sense to keep these next to getRefreshTokenResult and getRefreshTokenResult. That's the pattern we've used elsewhere and keeps them alphabetized.

🤔 Instead of exposing this via TokenService, would it be better to pass ActiveAccountStateProvider (implemented by StateService) to AccountTokenProvider?

⛏️ Can you alphabetize these with the other properties?

⛏️ Can you alphabetize these methods?

🤔 Should these be alphabetized?